-
Notifications
You must be signed in to change notification settings - Fork 0
compiler: claude can we have a kubernetes reporter #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
holy |
| // Clean and recreate the output directory. | ||
| if root.exists() { | ||
| if root.is_dir() { | ||
| std::fs::remove_dir_all(root) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like it's not too bad in theory, but the .env files kind of invite you to edit them so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, maybe let's make people rm -rf <whatever> && amber --k8s <whatever>. not that bad compared to overwriting a .env, which can be really annoying
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok ya
nhynes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks mostly reasonable. we should strongly consider whether it makes sense to include the cni checker which is more weighty than anticipated. an ignored rust test that exercises the new reporter in KinD and runs in CI would be cool. also a hoisting and cleanup pass. I'll run this through codex for completeness
| #[arg(long = "disable-networkpolicy-check", requires = "kubernetes")] | ||
| disable_networkpolicy_check: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. So far I haven't had a good answer for how to add reporter-specific args. Thanks for making a movement on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks claude
| // Clean and recreate the output directory. | ||
| if root.exists() { | ||
| if root.is_dir() { | ||
| std::fs::remove_dir_all(root) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, maybe let's make people rm -rf <whatever> && amber --k8s <whatever>. not that bad compared to overwriting a .env, which can be really annoying
| .collect::<Vec<_>>() | ||
| .join(" -> "); | ||
| return Err(ReporterError::new(format!( | ||
| "kubernetes reporter requires an acyclic dependency graph (ignoring weak bindings). \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this error shouldn't be some non-string error or something since the compose reporter also prefers acyclic. I also wonder if not all scenarios should be required to be acyclic, hoisting the check to scenario construction (linking) time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
give specific error variant: ok;
non-string: 🤷 ;
hoist check: in favor;
hoist check to link time: maybe, if all possible amber use cases need it;
are all amber scenarios acyclic: can't commit to this, although workaround if you need a cycle is to deploy a socket wrapper to connect in the opposite direction in your containers
| Ok(endpoint.port) | ||
| } | ||
|
|
||
| fn compose_templates_dfs( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blatant plagiarism
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gonna add you as Co-authored-by
|
|
||
| /// Try to resolve a config query against a composed template. | ||
| /// Returns Static if the value is fully resolved, Runtime if it contains config refs. | ||
| fn resolve_config_query_for_program( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR could use a pass to collect all of the shared code into one place. I can nominate myself for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, wanna try?
| /// - Phase 1: Try to connect to allowed server. If this fails, networking is broken - exit with error. | ||
| /// - Phase 2: Try to connect to blocked server. If this succeeds, NetworkPolicy isn't enforced - exit with error. | ||
| /// - Success: Both checks pass, stay alive. | ||
| fn generate_netpol_enforcement_check( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we really care that much that we would add 300 lines of code for it. This should be replaced at some point by L7 enforcement, so maybe we don't bother
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya shall we just get rid of all this
| miette = { workspace = true, features = ["derive"] } | ||
| serde = { workspace = true, features = ["derive"] } | ||
| serde_json = { workspace = true } | ||
| serde_yaml = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
controversial opinion: yaml is a superset of json. write out json instead of bringing in yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok on json
| - `linker`: schema validation, binding resolution, and export verification. | ||
| - `passes`: graph rewrites that must preserve scenario invariants. | ||
| - `reporter`: transforms `CompileOutput` into artifacts (e.g., scenario IR JSON, DOT, Docker Compose YAML). | ||
| - `reporter`: transforms `CompileOutput` into artifacts (e.g., scenario IR JSON, DOT, Docker Compose YAML, Kubernetes YAML). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and then leave it described as kubernetes YAML to mess with people
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah on labeling it as yaml 😆
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da969e8784
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Vague suggestion: docker-compose and k8s work in much the same way with programs and sidecars being turned into a "workload" and then rendered. There's a lot of duplicated code to prove this point. Would it be valuable to move the shared logic into one or more compose+k8s passes so that they can both be thin renderers on top of the same postprocessed IR? If you don't think that makes sense to put in this PR, I'm happy to try a refactoring pass after it's in. |
|
ya leme try to get this stuff settled before drying |
Co-authored-by: Nick Hynes <nhynes@nhynes.com>
Co-authored-by: Nick Hynes <nhynes@nhynes.com>
da969e8 to
c8022d3
Compare
|
big things not yet undertaken by myself:
|
|
can I merge this |
does it work?
|
|
oh yeah gotta add that
…On Tue, Jan 27, 2026, 9:58 PM Nick Hynes ***@***.***> wrote:
*nhynes* left a comment (RDI-Foundation/amber#3)
<#3 (comment)>
can I merge this
does it work? I can't find a KinD test
—
Reply to this email directly, view it on GitHub
<#3 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJDVILW3IIUQMLSD2BFT7RT4JBFYNAVCNFSM6AAAAACSPFYLO2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTQMBZGE3DCOJVGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
Disclaimer: This e-mail and any attachments may contain confidential
information. If you are not the intended recipient, any disclosure,
copying, distribution or use of any information contained herein is
strictly prohibited. If you have received this transmission in error,
please immediately notify the sender and destroy the original transmission
and any attachments without reading or saving.
|
|
adding e2e test for kubernetes reporter with KinD. dunno how to do it as a rust test |
|
passes on ci, can I merge this |
|
thanks, "squash and merge" ing |
--kubernetes <output dir>writes a directory of kubernetes config files and a kustomization.yaml. usekubectl apply -k <ouptut_dir>to put up the scenarioamber-root-configConfigMap out of itamber-root-configvalues into places OR use the amber-compose-helper to exec a program with string interpolations using those values